Add AvailableBalances::next_splice_out_maximum_sat#4550
Add AvailableBalances::next_splice_out_maximum_sat#4550tankyleo wants to merge 5 commits intolightningdevkit:mainfrom
AvailableBalances::next_splice_out_maximum_sat#4550Conversation
|
👋 Thanks for assigning @TheBlueMatt as a reviewer! |
tankyleo
force-pushed
the
2026-04-next-splice-out-maximum
branch
from
724acf9 to
3bd7a76
Compare
|
I've completed a thorough review of every file and hunk in the diff, cross-referencing with the surrounding codebase. All significant issues were already identified in my prior review pass. Review SummaryNo new bugs or security issues found beyond what was previously flagged. Previously Flagged Issues (Still Valid)These remain from the prior review:
Previously Flagged Issues (Corrected)My prior inline comments about unit mismatches in Cross-Cutting ConcernStale naming after semantic change: The value stored in |
tankyleo
requested review from
TheBlueMatt and
wpaulino
and removed request for
jkczyz
|
Seems the fuzz failure produces this error on my machine Hex: Target: chanmon_consistency Error: |
|
🔔 1st Reminder Hey @TheBlueMatt @wpaulino! This PR has been waiting for your review. |
1 similar comment
|
🔔 1st Reminder Hey @TheBlueMatt @wpaulino! This PR has been waiting for your review. |
|
Curious about |
Thanks for taking a look ! 183sats is the amount allocated to the fee of the splice out transaction at the default feerate of 253sat/kw, and will be added on top of the splice amount. Will clarify this in the test. |
|
🔔 2nd Reminder Hey @TheBlueMatt @wpaulino! This PR has been waiting for your review. |
1 similar comment
|
🔔 2nd Reminder Hey @TheBlueMatt @wpaulino! This PR has been waiting for your review. |
| their_contribution_candidate: SignedAmount, addl_nondust_htlc_count: usize, | ||
| feerate_per_kw: u32, | ||
| ) -> Result<(Amount, Amount), String> { | ||
| let htlc_candidate = None; |
There was a problem hiding this comment.
This really feels like it could/should be substantially DRYd with get_next_local/remote_commitment_stats - what we're really asking at the callsites is "is this splice even valid" which istm should really be done by just saying "did get_next_local_commitment_stats return an Error or not" (with appropriate balance modification flags, maybe a wrapper method calling an internal impl method rather than modifying the existing methods).
| /// retains at least one output after the splice, which is particularly relevant for | ||
| /// zero-reserve channels. | ||
| // | ||
| // The equation to determine `percent_max_splice_out_sat` is: |
There was a problem hiding this comment.
discussed offline but this feels super unclear to me because "percent_" as a prefix implies to me, that the value is a percent, but its not.
|
Needs rebase, sorry about that. Do think its worth doing this for 0.3, yes, given it fixed splice <-> 0 reserve compat. Its also not a big deal to add a field rn. |
Makes sense, thanks for clarifying! |
|
🔔 3rd Reminder Hey @wpaulino! This PR has been waiting for your review. |
tankyleo
force-pushed
the
2026-04-next-splice-out-maximum
branch
from
3bd7a76 to
245edbe
Compare
| } | ||
| max_splice_out_sat | ||
| } else { | ||
| // In a zero-reserve channel, the holder is free to withdraw up to its `post_splice_delta_above_reserve_sat` |
There was a problem hiding this comment.
Nit: This comment is backwards — it reads as if post_splice_delta_above_reserve_sat is the maximum the holder can withdraw, but the code computes local_balance - delta, meaning the holder can withdraw everything except the delta. Consider:
| // In a zero-reserve channel, the holder is free to withdraw up to its `post_splice_delta_above_reserve_sat` | |
| // In a zero-reserve channel, the holder is free to withdraw up to its balance minus `post_splice_delta_above_reserve_sat` |
tankyleo
force-pushed
the
2026-04-next-splice-out-maximum
branch
from
245edbe to
bb91e11
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4550 +/- ##
==========================================
+ Coverage 86.99% 87.02% +0.02%
==========================================
Files 163 161 -2
Lines 109008 109019 +11
Branches 109008 109019 +11
==========================================
+ Hits 94828 94870 +42
+ Misses 11696 11662 -34
- Partials 2484 2487 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
tankyleo
force-pushed
the
2026-04-next-splice-out-maximum
branch
from
bb91e11 to
ccde0da
Compare
TheBlueMatt
left a comment
TheBlueMatt
left a comment
There was a problem hiding this comment.
A few comments but largely LGTM, we can also address comments in a followup.
| .get_holder_counterparty_balances_floor_incl_fee(&self.funding) | ||
| .map(|(h, _)| h) | ||
| .ok(); | ||
| let holder_balance = self.get_next_splice_out_maximum(&self.funding).ok(); |
There was a problem hiding this comment.
We need to update the docs in funding.rs to match this, and maybe even rename the variables through to there.
There was a problem hiding this comment.
@wpaulino WDYT ? perhaps in that module it makes sense to think of the "holder's balance" as "the maximum they can splice out of the channel".
There was a problem hiding this comment.
Yeah I think that's fine, could also refer to it as the "spliceable balance" if we don't want to explicitly call out "splice out"
| .saturating_add(100) | ||
| .saturating_sub(post_splice_delta_above_reserve_sat.saturating_mul(100)) | ||
| .saturating_sub(channel_value_satoshis); | ||
| let max_splice_percentage_constraint_sat = dividend.saturating_sub(1) / 99; |
There was a problem hiding this comment.
The -1 doesn't appear in the math above?
There was a problem hiding this comment.
Added a comment to clarify this, see below
| let next_splice_out_maximum = self.get_next_splice_out_maximum(&self.funding)?; | ||
| let unsigned_contribution = our_funding_contribution.unsigned_abs(); | ||
| next_splice_out_maximum.to_sat().checked_add_signed(our_funding_contribution.to_sat()) | ||
| .ok_or(format!("Our splice-out value of {unsigned_contribution} is greater than the maximum {next_splice_out_maximum}")) |
There was a problem hiding this comment.
nit: can we debug assert that validate_splice_contributions passes? It has nice additional debug assertions i think we should try to retain.
There was a problem hiding this comment.
when we call get_next_splice_out_maximum we run validate_splice_contributions with the maximum applied and check that it passes
tankyleo
force-pushed
the
2026-04-next-splice-out-maximum
branch
from
ccde0da to
fb777b8
Compare
| // TODO: Skip 0FC channels for now as these always have an output on the commitment, the P2A | ||
| // output. We will be able to withdraw up to the dust limit of the funding script, which | ||
| // is checked in interactivetx. Still need to double check whether that's what we actually | ||
| // want. |
There was a problem hiding this comment.
@TheBlueMatt forgot to mention this TODO here, WDYT ?
|
diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs
index 8931b05bc..5b165a28b 100644
--- a/lightning/src/ln/channel.rs
+++ b/lightning/src/ln/channel.rs
@@ -12401,8 +12401,8 @@
"build_prior_contribution requires pending_splice"
);
let prior = self.pending_splice.as_ref()?.contributions.last()?;
- let holder_balance = self.get_next_splice_out_maximum(&self.funding).ok();
- Some(PriorContribution::new(prior.clone(), holder_balance))
+ let next_splice_out_maximum = self.get_next_splice_out_maximum(&self.funding).ok();
+ Some(PriorContribution::new(prior.clone(), next_splice_out_maximum))
}
/// Returns whether this channel can ever RBF, independent of splice state.
@@ -12476,13 +12476,13 @@
return contribution;
}
- let holder_balance = match self.get_next_splice_out_maximum(&self.funding) {
+ let next_splice_out_maximum = match self.get_next_splice_out_maximum(&self.funding) {
Ok(balance) => balance,
Err(_) => return contribution,
};
- if let Err(e) =
- contribution.net_value_for_initiator_at_feerate(min_rbf_feerate, holder_balance)
+ if let Err(e) = contribution
+ .net_value_for_initiator_at_feerate(min_rbf_feerate, next_splice_out_maximum)
{
log_info!(
logger,
@@ -12503,7 +12503,7 @@
min_rbf_feerate,
);
contribution
- .for_initiator_at_feerate(min_rbf_feerate, holder_balance)
+ .for_initiator_at_feerate(min_rbf_feerate, next_splice_out_maximum)
.expect("feerate compatibility already checked")
}
@@ -12876,7 +12876,7 @@
fn resolve_queued_contribution<L: Logger>(
&self, feerate: FeeRate, logger: &L,
) -> Result<(Option<SignedAmount>, Option<Amount>), ChannelError> {
- let holder_balance = self
+ let next_splice_out_maximum = self
.get_next_splice_out_maximum(&self.funding)
.map_err(|e| {
log_info!(
@@ -12889,9 +12889,12 @@
})
.ok();
- let net_value = match holder_balance.and_then(|_| self.queued_funding_contribution()) {
+ let net_value = match next_splice_out_maximum
+ .and_then(|_| self.queued_funding_contribution())
+ {
Some(c) => {
- match c.net_value_for_acceptor_at_feerate(feerate, holder_balance.unwrap()) {
+ match c.net_value_for_acceptor_at_feerate(feerate, next_splice_out_maximum.unwrap())
+ {
Ok(net_value) => Some(net_value),
Err(FeeRateAdjustmentError::FeeRateTooHigh { .. }) => {
return Err(ChannelError::Abort(AbortReason::FeeRateTooHigh));
@@ -12911,7 +12914,7 @@
None => None,
};
- Ok((net_value, holder_balance))
+ Ok((net_value, next_splice_out_maximum))
}
pub(crate) fn splice_init<ES: EntropySource, L: Logger>(
diff --git a/lightning/src/sign/tx_builder.rs b/lightning/src/sign/tx_builder.rs
index d78f65d22..ade54c603 100644
--- a/lightning/src/sign/tx_builder.rs
+++ b/lightning/src/sign/tx_builder.rs
@@ -324,7 +324,7 @@
//
// The equation to determine `max_splice_percentage_constraint_sat` is:
// 1) floor((c - s) / 100) == h - s - d
-// We want the maximum value of s that will satisfy this equation, therefore, we solve:
+// We want the maximum value of s that will satisfy equation 1, therefore, we solve:
// 2) (c - s) / 100 < h - s - d + 1
// where c: `channel_value_satoshis`
// s: `max_splice_percentage_constraint_sat`
@@ -343,16 +343,17 @@
.counterparty_selected_channel_reserve_satoshis
!= 0
{
- let dividend = local_balance_before_fee_sat
+ let dividend_sat = local_balance_before_fee_sat
.saturating_mul(100)
.saturating_add(100)
.saturating_sub(post_splice_delta_above_reserve_sat.saturating_mul(100))
.saturating_sub(channel_value_satoshis);
- let max_splice_percentage_constraint_sat = dividend.saturating_sub(1) / 99;
+ // Calculate the greatest integer that is strictly less than the RHS of inequality 3 above
+ let max_splice_percentage_constraint_sat = dividend_sat.saturating_sub(1) / 99;
let max_splice_dust_limit_constraint_sat = local_balance_before_fee_sat
.saturating_sub(channel_constraints.holder_dust_limit_satoshis)
.saturating_sub(post_splice_delta_above_reserve_sat);
- // Take whichever constraint you hit first as you increase the value of the splice-out
+ // Both constraints must be satisfied, so take the minimum of the two maximums
let max_splice_out_sat =
cmp::min(max_splice_percentage_constraint_sat, max_splice_dust_limit_constraint_sat);
#[cfg(debug_assertions)] |
|
Needs a rebase |
As a result, we now validate that both commitments retain at least one output under the new funding scope, which is crucial for zero-reserve channels.
We previously determined this value by subtracting the htlcs, the anchors, and the commitment transaction fee. This ignored the reserve, as well as the at-least-one-output requirement in zero-reserve channels. This new field now accounts for both of these constraints. It can be seen as the total spliceable balance from the channel.
This is equivalent to the previous commit, see the debug assertions added in the previous commit. We now also get to communicate the exact maximum back to the user, instead of some "balance is lower than our reserve" message, which is hard to react to.
tankyleo
force-pushed
the
2026-04-next-splice-out-maximum
branch
from
fb777b8 to
d3b1c31
Compare
6 participants
This is the maximum value the holder can splice out of their channel balance, accounting for the updated v2 reserves after the splice, and the "must have at least one output" constraint on zero-reserve channels.